Add checkpoint primitives #81
Conversation
| function getCheckpointHash(AccessLogs.Context memory a) | ||
| internal | ||
| pure | ||
| returns (bytes32) | ||
| { | ||
| return a.readLeaf( | ||
| Memory.strideFromLeafAddress( | ||
| EmulatorConstants.checkpointAddress.toPhysicalAddress() | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think this function is returning the hash of the checkpoint hash. Can you verify?
There was a problem hiding this comment.
If so, the procedure should be something like:
- call
readLeaf, store result atL; - consume another bytes32, which will be the "alleged" checkpoint hash itself. Store result in
V; - check that the keccak of
VmatchesL; - return
V.
There was a problem hiding this comment.
You are right, I totally missed that.
| function setCheckpointHash( | ||
| AccessLogs.Context memory a, | ||
| bytes32 checkPointHash | ||
| ) internal pure { | ||
| a.writeLeaf( | ||
| Memory.strideFromLeafAddress( | ||
| EmulatorConstants.checkpointAddress.toPhysicalAddress() | ||
| ), | ||
| checkPointHash | ||
| ); | ||
| } |
There was a problem hiding this comment.
Likewise, I think this is writing the hash of the checkpoint hash. Can you verify?
There was a problem hiding this comment.
If so, this function should keccak the checkpointHash before writing it.
|
|
||
| uint32 constant IFLAGS_Y_SHIFT = 1; | ||
| uint64 constant LOG2_CYCLES_TO_RESET = 10; | ||
| uint64 constant checkpointAddress = 0x7ffff000; |
There was a problem hiding this comment.
We should use screaming snake case for constants, as the other constants!
src/AdvanceStatus.sol
Outdated
| } | ||
|
|
||
| library AdvanceStatus { | ||
| // START OF AUTO-GENERATED CODE |
There was a problem hiding this comment.
It's not but I supposed it'll be sometimes in the future. Would you like it to be removed?
There was a problem hiding this comment.
Yeah, I think if it's not auto generated, we should remove it.
src/AdvanceStatus.sol
Outdated
| uint64 tohost = | ||
| EmulatorCompat.readWord(a, EmulatorConstants.HTIF_TOHOST_ADDRESS); | ||
| uint16 reason = uint16((tohost >> 16) & ((1 << 16) - 1)); |
There was a problem hiding this comment.
Add comments about the endianness, and layout of the tohost register:
typedef struct cmt_io_yield {
uint8_t dev;
uint8_t cmd;
uint16_t reason;
uint32_t data;
} cmt_io_yield_t;
src/AdvanceStatus.sol
Outdated
| enum Status { | ||
| NOT_YIELDED, | ||
| ACCEPTED, | ||
| REJECTED, | ||
| EXCEPTION | ||
| } |
There was a problem hiding this comment.
We should move this type inside library AdvanceStatus so that it becomes "scoped" to that library.
|
Just checked the new commits, looks great to me. I've added an additional style comment. I'd love for @mpernambuco to take a look as well. Note for reviewers, extracting the yield |
stephenctw
force-pushed
the
feature/add-checkpoint-primitives
branch
2 times, most recently
from
d0f127f to
31de9b1
Compare
src/EmulatorCompat.sol
Outdated
| ) | ||
| ); | ||
| assert(keccak256(abi.encodePacked(checkpointHash)) == hashOfCheckpointHash); | ||
| assert( |
There was a problem hiding this comment.
I missed this: I think this is a require and not an assert.
stephenctw
force-pushed
the
feature/add-checkpoint-primitives
branch
from
31de9b1 to
2753737
Compare
stephenctw
force-pushed
the
feature/add-checkpoint-primitives
branch
from
2753737 to
96a22c9
Compare
stephenctw
force-pushed
the
feature/add-checkpoint-primitives
branch
from
289984a to
e8050dd
Compare
MANUAL_REASONAdvanceStatus.solCheckpointgetter/setter